Add promises#81
Conversation
|
First things first, looks like we need to get the test suite running on Travis again. |
|
The grunt fix here was merged as 25bfc26. |
arlolra
left a comment
arlolra
left a comment
There was a problem hiding this comment.
In general, you probably want to reject promises in the those functions, rather than call notify.
Also, this could use a rebase.
| this.storedMgs.push({msg: msg, meta: meta}) | ||
| this.sendQueryMsg() | ||
| return | ||
| return promise |
There was a problem hiding this comment.
This promise is never rejected or fulfilled, similarly all the cases below where the functions return early.
There was a problem hiding this comment.
This was by purpose, to resolve a Promise only if there were some data to send. But yes in this case sendQueryMsg should return a promise and also notify should return an rejected promise. Will update the pr accordingly.
| msg = this.prepareMsg(msg) | ||
| break | ||
| default: | ||
| throw new Error('Unknown message state.') |
There was a problem hiding this comment.
Throwing an exception is the same as rejecting.
| case 'error': | ||
| this.notify(msg.msg) | ||
| return | ||
| return promise |
There was a problem hiding this comment.
return promise.reject(msg.msg), no?
|
I just encountered that I have to add promises to a lot more functions, like |
|
Ok, there may be some hints in #75 which promisifies some portion of the code.
Yes, adding to the test suite would be beneficial. |
|
@sualko Concerning a polyfill, you can try es6-promise. |
|
Before I continue, we have to decide when The aim of promises in this case (at least for me) is to get a relation between a plaintext and encrypted message, which is ensured in the current pr. |
3 participants
I did just some quick tests, but I think it needs some more exhaustive ones.